Add more context handling #139
Conversation
njvrzm
force-pushed
the
njvrzm/refactor_and_add_context
branch
from
2832cfa to
b14075d
Compare
| @@ -13,17 +13,17 @@ steps: | |||
| - name: build | |||
| image: grafana/grafana-plugin-ci:1.9.5 | |||
| commands: | |||
| - mage -v build | |||
There was a problem hiding this comment.
Some of the build steps were randomly failing, I believe because they share a docker volume and one step's mage process is deleting the mage_output_file.go before another step is done with it. This change appears to fix it.
| GetDB(ctx context.Context, id int64, options sqlds.Options) (*sql.DB, error) | ||
| GetAsyncDB(ctx context.Context, id int64, options sqlds.Options) (awsds.AsyncDB, error) | ||
| GetAPI(ctx context.Context, id int64, options sqlds.Options) (api.AWSAPI, error) | ||
| } |
There was a problem hiding this comment.
Athena and redshift were defining this interface for themselves, which seemed backwards to me; rather than update both copies with context arguments I've moved it here.
| LoadAPI(context.Context, *awsds.SessionCache, models.Settings) (api.AWSAPI, error) | ||
| LoadDriver(context.Context, api.AWSAPI) (driver.Driver, error) | ||
| LoadAsyncDriver(context.Context, api.AWSAPI) (asyncDriver.Driver, error) | ||
| } |
There was a problem hiding this comment.
I don't love this approach but I like it better than passing around a bunch of typed functions. I'm open to alternatives :)
There was a problem hiding this comment.
I don't think its better (probably equivalent tbh) but you could make a struct of the options and pass that around?
There was a problem hiding this comment.
That would work! I think you're right though that it's not really better. Gonna keep it this way since it's already done.
| LoadAPI(context.Context, *awsds.SessionCache, models.Settings) (api.AWSAPI, error) | ||
| LoadDriver(context.Context, api.AWSAPI) (driver.Driver, error) | ||
| LoadAsyncDriver(context.Context, api.AWSAPI) (asyncDriver.Driver, error) | ||
| } |
There was a problem hiding this comment.
I don't think its better (probably equivalent tbh) but you could make a struct of the options and pass that around?
For compatibility with grafana-plugin-sdk (specifically changes in v0.177.0 and v0.182.0) we're adding
context.Contextto more places. This also brings in an interface that two client datasources (athena and redshift) have been defining identically for themselves, and streamlines the various loaders that were being passed around into a single interface thatdatasource.Newwill expect. Finally,context.TODO()is replaced withcontext.Background()in tests.This is a breaking change, so the next release should be >= v0.26.